fix(ui): read SwapKit's error codes on the DEX amount screens - #1087
fix(ui): read SwapKit's error codes on the DEX amount screens#1087romchornyi wants to merge 2 commits into
Conversation
Every amount-related failure on Enter amount rendered as a dead end
("Something went wrong setting up your swap") even when SwapKit had said
exactly what was wrong. Two causes, both about where the code was read
from.
`decodeQuoteError` returned `body.message` before `body.error`, so the
prose reached the mapper and the code never did: "No routes found for
DASH.DASH -> BTC.BTC" does not match `noRoutesFound`, and the buy
screen's own `contains("noRoutesFound")` check missed it for the same
reason. And a failure a provider reports — an HTTP 200 with no routes
and `providerErrors[]` — was passed on as its prose alone, dropping
`errorCode`, the only stable identifier in the response.
Measured against api.swapkit.dev on 2026-08-28: at 0.05 DASH a
DASH -> BTC quote answers 200 with `sellAssetAmountTooSmall`; below
about 0.01 DASH it answers 404 `noRoutesFound`; above the pool's depth
it answers 200 with `apiRequestFailed`. All three read as "something
went wrong".
- `providerErrorMessage` puts the code back in front of the prose, so
provider-level and top-level failures reach the mapper the same way.
- Below-minimum codes are family-matched on an `AmountTooSmall` /
`AmountTooLow` suffix rather than enumerated, since SwapKit does not
document the per-provider vocabulary.
- `apiRequestFailed`, `invalidRoute`, `invalidAsset` and
`memoTooLongForSourceChain` are mapped; the last shares the copy the
wallet already uses when it catches an over-length memo locally.
- Routability probing classifies on codes too: a below-minimum reply
says the probe amount was too small, not that the asset is unroutable,
so the coin picker no longer hides an asset on that evidence.
- The convert screen's over-balance line follows the redesign to
"Max $205.32" (Figma 24034:44864).
Mirrors Android's `SwapKitErrors` (dashpay/dash-wallet#1526) and the
enter-amount copy from dashpay/dash-wallet#1539.
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSwapKit errors now use normalized, case-insensitive codes for provider formatting, route classification, and amount validation. Buy and convert flows use shared error mapping. Localization adds below-minimum and shortened maximum-amount messages. ChangesSwapKit error flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR improves how existing SwapKit errors are classified and shown on swap amount screens without changing transaction authority, stored state, or external interfaces; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant SwapKitSwapProvider
participant SwapKitErrorCopy
participant BuyEnterAmountViewModel
participant SwapConvertViewModel
SwapKitSwapProvider->>SwapKitErrorCopy: Normalize provider error
SwapKitErrorCopy-->>SwapKitSwapProvider: Return error code and routability
SwapKitSwapProvider->>BuyEnterAmountViewModel: Return quote error
BuyEnterAmountViewModel->>SwapKitErrorCopy: Map validation message
SwapKitSwapProvider->>SwapConvertViewModel: Return API error
SwapConvertViewModel->>SwapKitErrorCopy: Map conversion message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 4 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🕓 Ready for review — 28 ahead in queue (commit 46ebf78) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — GLM Flash + Sol
The code-first normalization correctly preserves stable SwapKit codes and improves the amount-screen copy. Two in-scope suggestions remain: top-level noRoutesFound is still treated as conclusive despite the PR's measured amount ambiguity, and the new normalization and classification behavior lacks regression coverage; two minor documentation and cleanup issues are also noted.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-5.6-sol (agent: phase2-reviewer, role: general); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
Review provenance
- Phase 1 reviewers (GLM Flash):
glm-5.3-flash— general (completed); agentphase1-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Phase 2 reviewers (Sol):
gpt-5.6-sol— general (completed); agentphase2-reviewer
🟡 2 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift`:
- [SUGGESTION] DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift:749-751: Do not classify top-level `noRoutesFound` as conclusively unroutable
The PR's recorded API behavior shows that SwapKit returns top-level `noRoutesFound` when an amount is far below a route's minimum. This branch nevertheless caches `.notRoutable` for ten minutes, so an asset can disappear from the optimistic Buy picker when its approximately $50 probe—or the one-unit fallback—falls below that asset's route floor. Remove this branch and let the response fall through to `nil`; explicit provider below-minimum errors can remain positive routability evidence, while actual routes remain conclusive positive proof.
In `DashWallet/Sources/Models/Swap/SwapKitErrorCopy.swift`:
- [SUGGESTION] DashWallet/Sources/Models/Swap/SwapKitErrorCopy.swift:40-79: Add regression tests for the new SwapKit error normalization
This bug fix introduces several interacting parsing and classification rules without executable coverage. Extend the existing SwapKit tests with compile-ready cases for provider code/detail composition, code-only and detail-only responses, whitespace and nil handling, code-first top-level decoding, case-insensitive `AmountTooSmall` and `AmountTooLow` suffixes, the four newly mapped codes, unknown-code fallback, and routability outcomes for explicit below-minimum versus ambiguous `noRoutesFound` responses. These tests are needed to prevent a decoder or mapper refactor from dropping the stable code behind provider prose again.
- [NITPICK] DashWallet/Sources/Models/Swap/SwapKitErrorCopy.swift:38: Correct the nonexistent mapper reference in the doc comment
The comment references `message(for:coin:minimum:)`, but this module only defines `message(for:coin:)`. Point the documentation at the actual mapper so readers are not directed to a nonexistent overload.
- [NITPICK] DashWallet/Sources/Models/Swap/SwapKitErrorCopy.swift:55-65: Remove the unused `isAmountTooLow(_:)` helper
Repository-wide usage shows no caller for this newly added helper. The amount screens call `message(for:coin:)`, while routability intentionally uses the narrower `isNoRoute(_:)` and `isBelowMinimum(_:)` methods. Delete the unused API and move any useful ambiguity explanation to the classifier that implements the behavior.
| if SwapKitErrorCopy.isNoRoute(response.error) { | ||
| return .notRoutable | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Do not classify top-level noRoutesFound as conclusively unroutable
The PR's recorded API behavior shows that SwapKit returns top-level noRoutesFound when an amount is far below a route's minimum. This branch nevertheless caches .notRoutable for ten minutes, so an asset can disappear from the optimistic Buy picker when its approximately $50 probe—or the one-unit fallback—falls below that asset's route floor. Remove this branch and let the response fall through to nil; explicit provider below-minimum errors can remain positive routability evidence, while actual routes remain conclusive positive proof.
source: ['codex']
There was a problem hiding this comment.
Fixed in 46ebf78 — the top-level branch is gone and that response now falls through to nil.
Agreed on the reasoning, and the recorded data is worse than the comment suggests: DASH → BTC returns top-level noRoutesFound at 0.01 DASH and a route at 0.3, so the ambiguous band is wide. The probe’s one-unit fallback (no cached USD price) lands inside it for any cheap asset.
Provider-level evidence still decides: a below-minimum code is positive, a provider naming its own noRoutesFound in providerErrors[] is the one conclusive negative — it answers for the single provider the probe asked about — and anything else (apiRequestFailed) leaves the question open. The optimistic-filter comment above candidates.filter was promising pruning the top-level code no longer performs, so it was corrected too.
| static func providerErrorMessage(_ error: SwapKitProviderError?) -> String? { | ||
| let code = error?.errorCode?.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let detail = error?.message?.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| switch (code?.isEmpty == false ? code : nil, detail?.isEmpty == false ? detail : nil) { | ||
| case let (code?, detail?): | ||
| return "\(code): \(detail)" | ||
| case let (code?, nil): | ||
| return code | ||
| case let (nil, detail?): | ||
| return detail | ||
| case (nil, nil): | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| /// True when the failure means "the sell amount is under what this route can fill" — the case | ||
| /// the amount screens surface inline (raise the amount and retry) instead of as a dead end. | ||
| /// `noRoutesFound` is included because SwapKit answers with it for amounts far below the | ||
| /// minimum and only switches to an explicit below-minimum code close to the floor: measured | ||
| /// 2026-08-28, DASH → BTC returned `noRoutesFound` at 0.01 DASH and `sellAssetAmountTooSmall` | ||
| /// (min 0.175) at 0.05. It is genuinely ambiguous — a route can also be briefly unavailable — | ||
| /// so its copy stays neutral. | ||
| static func isAmountTooLow(_ rawError: String?) -> Bool { | ||
| let code = code(of: rawError) | ||
| return code == noRoutesFoundCode || isBelowMinimumCode(code) | ||
| } | ||
|
|
||
| static func message(for rawError: String?, coin: SwapCryptoCurrency) -> String { | ||
| let code = rawError? | ||
| .components(separatedBy: ":") | ||
| .first? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| .lowercased() | ||
| ?? "" | ||
| let code = code(of: rawError) | ||
|
|
||
| // Per-provider below-minimum codes are family-matched rather than enumerated: SwapKit does | ||
| // not document the per-provider vocabulary, and the prefix names whichever side was too | ||
| // small (`sellAssetAmountTooSmall` today). Both forms carry "amount", so an unrelated | ||
| // below-threshold code — a too-low fee, say — stays out. | ||
| if isBelowMinimumCode(code) { | ||
| return NSLocalizedString( | ||
| "This amount is below the minimum for this swap. Please enter a larger amount.", | ||
| comment: "Dash DEX / dex_error_amount_too_small" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add regression tests for the new SwapKit error normalization
This bug fix introduces several interacting parsing and classification rules without executable coverage. Extend the existing SwapKit tests with compile-ready cases for provider code/detail composition, code-only and detail-only responses, whitespace and nil handling, code-first top-level decoding, case-insensitive AmountTooSmall and AmountTooLow suffixes, the four newly mapped codes, unknown-code fallback, and routability outcomes for explicit below-minimum versus ambiguous noRoutesFound responses. These tests are needed to prevent a decoder or mapper refactor from dropping the stable code behind provider prose again.
source: ['claude', 'codex']
There was a problem hiding this comment.
Added in 46ebf78 — a SwapKitErrorCopyTests case alongside the existing decoding tests, 12 methods covering: code-before-prose composition and its code-only / detail-only / blank / nil fallbacks; the case-insensitive AmountTooSmall / AmountTooLow suffix family, including the composed "<code>: <detail>" form and a negative for inboundFeeTooLow; isNoRoute on both forms; the four newly mapped codes; memoTooLongForSourceChain sharing the local memo copy; detail-after-code not changing the mapping; unknown and empty falling back to generic.
The anchor case is the regression itself: message(for: "No routes found for DASH.DASH -> BTC.BTC") must still be generic. The fix is that callers stopped sending prose alone, not that the mapper started guessing from text — a test asserting the prose maps would cement exactly the wrong behaviour.
Two limits worth stating plainly:
- Not executed. The test target cannot be built on this machine: the
../platformcheckout it takes SwiftDashSDK from sits on a side branch that predates several symbolsdevelopneeds (ManagedPlatformWallet.ParsedIdentityUpdateTransition,PlatformWalletManager.trackedMasternodes,MasternodeKeyRole), andDashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swiftreferences the first of those directly. That is unrelated to this PR and pre-dates it. To get a real signal I extractedSwapKitErrorCopyverbatim into a standalone target with stubbedSwapKitProviderError/SwapCryptoCurrency/DWLoggerand ran the same 28 assertions these tests encode — all pass. Whoever has an up-to-date../platformshould run the XCTest cases before merge. - Two call sites stay covered only indirectly.
routability(from:)anddecodeQuoteError(from:)are private, and reaching them would mean widening the access of a private nested enum and a method purely for tests. Every rule they depend on (providerErrorMessage,isNoRoute,isBelowMinimum, code-prefix extraction) is tested directly, so a refactor that puts prose back in front of the code fails here. Say the word if you would rather have them internal and asserted end-to-end.
| static let noRoutesFoundCode = "noroutesfound" | ||
|
|
||
| /// A provider-level failure rendered in the same `"<code>: <detail>"` shape the top-level | ||
| /// `error` field uses, so both reach `message(for:coin:minimum:)` through one path. |
There was a problem hiding this comment.
💬 Nitpick: Correct the nonexistent mapper reference in the doc comment
The comment references message(for:coin:minimum:), but this module only defines message(for:coin:). Point the documentation at the actual mapper so readers are not directed to a nonexistent overload.
| /// `error` field uses, so both reach `message(for:coin:minimum:)` through one path. | |
| /// `error` field uses, so both reach `message(for:coin:)` through one path. |
source: ['codex']
There was a problem hiding this comment.
Fixed in 46ebf78. The same slip existed a second time — isBelowMinimum(_:) referred to [isAmountTooLow], which your other comment asked me to delete — so both now point at symbols that exist.
| /// True when the failure means "the sell amount is under what this route can fill" — the case | ||
| /// the amount screens surface inline (raise the amount and retry) instead of as a dead end. | ||
| /// `noRoutesFound` is included because SwapKit answers with it for amounts far below the | ||
| /// minimum and only switches to an explicit below-minimum code close to the floor: measured | ||
| /// 2026-08-28, DASH → BTC returned `noRoutesFound` at 0.01 DASH and `sellAssetAmountTooSmall` | ||
| /// (min 0.175) at 0.05. It is genuinely ambiguous — a route can also be briefly unavailable — | ||
| /// so its copy stays neutral. | ||
| static func isAmountTooLow(_ rawError: String?) -> Bool { | ||
| let code = code(of: rawError) | ||
| return code == noRoutesFoundCode || isBelowMinimumCode(code) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Remove the unused isAmountTooLow(_:) helper
Repository-wide usage shows no caller for this newly added helper. The amount screens call message(for:coin:), while routability intentionally uses the narrower isNoRoute(_:) and isBelowMinimum(_:) methods. Delete the unused API and move any useful ambiguity explanation to the classifier that implements the behavior.
source: ['codex']
There was a problem hiding this comment.
Removed in 46ebf78. It was added for parity with Android’s SwapKitErrors.isAmountTooLow, but nothing here calls it: the amount screens go through message(for:coin:) and routability deliberately uses the two narrower tests. The measured ambiguity it documented moved onto isNoRoute(_:), which is the classifier that acts on it.
Review follow-up. A top-level `noRoutesFound` was cached as a conclusive negative for ten minutes, but SwapKit answers with it for an amount far below a route's floor as well as for a pair it cannot carry — measured 2026-08-28, DASH -> BTC returned it at 0.01 DASH and quoted a route at 0.3. The probe is a $50 estimate that falls back to one whole unit when no USD price is cached, so a cheap asset could be probed under its own floor and disappear from the picker for the rest of the window. Only a provider naming its own no-route in `providerErrors[]` stays conclusive: it answers for the single provider the probe asked about. Also from review: drop `isAmountTooLow(_:)`, which had no callers once the amount screens moved to `message(for:coin:)` and routability took the two narrower tests; move the ambiguity it documented onto `isNoRoute(_:)`, which is what acts on it. Point two doc comments at symbols that exist. Adds regression coverage for the normalization rules: code-before-prose composition and its fallbacks, blank and nil handling, the case-insensitive below-minimum suffix family, the four newly mapped codes, the shared memo-too-long copy, unknown-code fallback, and the anchor case — prose without its code must not be mistaken for a mapping.
4 participants
Issue being fixed or feature implemented
Reported from the Dash DEX Enter amount screens: the errors SwapKit returns had changed, and the screen answered almost everything with the same dead end — "Something went wrong setting up your swap" — even when the API had said exactly what was wrong.
Verified by hand against
api.swapkit.devon 2026-08-28. An amount below a route's floor comes back three different ways depending on how far below it is:200+providerErrors[0].errorCode = sellAssetAmountTooSmall404+error = noRoutesFound200+providerErrors[0].errorCode = apiRequestFailedTwo reasons none of it reached the user:
decodeQuoteErrorreturnedbody.messagebeforebody.error, so the prose reached the mapper and the code never did."No routes found for DASH.DASH -> BTC.BTC"does not matchnoRoutesFound, and the buy screen's owncontains("noRoutesFound")check missed it for the same reason — so even the one error that screen tried to handle fell through to the generic copy.providerErrors[]entry — was passed on as its prose alone, droppingerrorCode, the only stable identifier in the response.What was done?
Mirrors Android's
SwapKitErrors(dashpay/dash-wallet#1526) and the enter-amount copy from dashpay/dash-wallet#1539.providerErrorMessage(_:)renders a provider failure as"<code>: <detail>", so provider-level and top-level failures reach the mapper through one path.AmountTooSmall/AmountTooLowsuffix rather than enumerated — SwapKit doesn't document the per-provider vocabulary, and the prefix names whichever side was too small. Both forms carry "Amount", so an unrelated below-threshold code (a too-low fee) stays out.apiRequestFailed,invalidRoute,invalidAsset,memoTooLongForSourceChain. The last one shares the copy the wallet already shows when it catches an over-length memo locally before broadcasting.noRoutesFoundstill counts as conclusive.decodeQuoteErroris code-first, matching what the swap-side decoder already did.maya_max_amount_error).One string is removed: the buy screen's hand-rolled
dex_enter_amount_invalidfallback is orphaned now that both amount screens share the mapper.Not in this PR
MAYACHAIN,MAYACHAIN_STREAMINGandTHORCHAINanswerednoRoutesFoundfor every pair tried on 2026-08-28 — includingBTC.BTC -> ETH.ETH— while/tokensstill lists 31 and 18 assets for the two Maya providers.NEARandCHAINFLIPwork. That is upstream, not ours; tracked separately.How Has This Been Tested?
dashpaybuild,ARCHS=arm64, iOS Simulator SDK —BUILD SUCCEEDED.api.swapkit.devwith the app's own API key and request bodies (quote and swap, both directions, amount sweeps per asset), and the mapping checked against those recorded responses./v3/swapanswersinvalidRoutefor DASH → BTC even with SwapKit's ownnextActionspayload).Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit